I wonder if sidecar_to_file_meta could be a closure. We only use this once.

let sidecar_to_filemeta = |sidecar| {
        let location = log_root.join("_sidecars/")?.join(&sidecar.path)?;
        Ok(FileMeta {
            location,
            last_modified: sidecar.modification_time,
            size: sidecar.size_in_bytes as usize,
        })
}

And then map sidecar

    visitor
            .sidecars
            .iter()
            .map(sidecar_to_filemeta)

Give it a shot and see how it is.

Do you think it's a good idea to leave it as a separate function for unit testing purposes?

I'd say either keep the separate function (if needed for testing) or embed the logic directly in the map call? What purpose does a separately named closure serve?

(aside: not sure if cargo fmt will like my indentation choice above -- depends on whether the ( or { is more important)

iirc, we want to avoid passing handlers around. Only reference to the engine. I think it's because we want to make it clear that the handler is tied to the engine and not to encourage holding an Arc ref to the handler.

cc @zachschuermann to double check.

I recall running into lifetime issues when passing the entire engine. I believe we would have to explicitly tie the iterator's lifetime to that of the engine?

You can also pass an Arc.

Basically the iterator needs to hold a reference for the entire duration it's lazily evaluating. So you want to give it a reference it can hold for a long time.

But this change extends all the way to changing the scan_data function signature to explicitly tie the engines lifetime to the iterator.

pub fn scan_data<'a>(
        &self,
        engine: &'a dyn Engine,
    ) -> DeltaResult<impl Iterator<Item = DeltaResult<ScanData>> + 'a> {

Mmm, so the basic issue is that we have delayed reading of parquet files, so at some point we want an item off the iterator, and to produce it, we need to read some parquet, so we need a handler. Previously we could do all the read calls up front and then just map off that iterator, so we didn't need an engine ref plumbed through.

I think if this is all internal, i.e., we don't want to expose any of these function signatures to engines (especially in the FFI), then cloning the Arcs is fine (it's very cheap. as a suggestion we usually put // cheap arc clone at those clone sites to make it clear).

If we do want to ever expose this, we'll need to think more, but afaict, we don't.

Thanks for the look @nicklan, just to confirm we will go ahead and clone the parquet handler

This looks wrong. Are we mishandling column nullability somewhere, that can cause empty strings to be returned instead of NULL?

It seems like this sort of issue has shown up a few times recently -- do we have a lurking bug somewhere? or is null handling just error-prone in general?

I encountered Sidecar actions with empty strings for path when running test_create_checkpoint_stream_reads_parquet_checkpoint_batch at first. I believe it is because of the way I am creating the dummy checkpoint batch (beginning with a json string, specifically add_batch_simple()). The fix is not obvious to me though...

For more context, when I create a dummy engine data batch from a json string (without sidecar actions) with the SyncEngine's json handler:

Below is the json string which is converted to engine data with string_array_to_engine_data, which is finally passed to the sync engine.

           r#"{"metaData":{"id":"testId","format":{"provider":"parquet","options":{}},"schemaString":"{\"type\":\"struct\",\"fields\":[{\"name\":\"value\",\"type\":\"integer\",\"nullable\":true,\"metadata\":{}}]}","partitionColumns":[],"configuration":{"delta.enableDeletionVectors":"true","delta.columnMapping.mode":"none"},"createdTime":1677811175819}}"#,

This code has disappeared in the latest version, did you figure out a fix?

I found a fix for the test case which created batches that had this weird empty-string Sidecar path field.

However, I am concerned that there may be something wrong with the SyncEngine's json handler's functionality as it allowed me to create this malformed batch

Sorry, how was the batch malformed (= physically invalid)?

It seemed like the test was simply passing an empty string, which is schema-compatible. Arrow has no way to know that Delta puts additional constraints on the field value?

Sorry for the confusion, the test case this issue originated from was: test_create_checkpoint_stream_reads_parquet_checkpoint_batch_without_sidecars I've left a comment below with more context

#692 solved this, right?

This is subtle -- replacing the top-level means all non-file actions will be lost. This is only safe correct if all checkpoint scans are exclusively requesting adds or exclusively requesting non-file actions. I'm pretty sure it will break our inspect-table example that visits all actions during log replay.

We would either need to keep returning the top-level actions unconditionally (safer) or inspect the read schema to see whether we need non-file actions. Simper feels (a lot) safer to me, and seems unlikely to cause any measurable performance hit -- each checkpoint part has thousands of actions, vs. dozens in the top level. manifest.

Based on the above, we might be able to eliminate multiple left/right use sites, by careful management of type signatures. Conceptually, we would always do a map over the top level checkpoint iterator, producing the following output:

let sidecar_content = Self::process_sidecars(top_level_batch, ...); // returns Option<impl Iterator>
std::iter::once(top_level_batch).chain(sidecar_content.into_iter().flatten())

We could pass a flag into process_sidecars that short circuits it to None (pretending no sidecars were found), or we could just cheat and do a spurious map call, just to get the correct signature:

std::iter::once(top_level_batch).chain(None.into_iter().flatten())

Ugh... compiler didn't like my toy example that did the spurious map call...

note: no two closures, even if identical, have the same type

thanks for the catch @scovich I was operating under the assumption that

all checkpoint scans are exclusively requesting adds or exclusively requesting non-file actions

would be always true. But keeping the top-level actions unconditionally feels like a much safer approach. I'll move forward with this and have noted this decision in the design doc.

This code has disappeared in the latest version, did you figure out a fix?

If you really wanted to get fancy, could do:

let top_iterable = need_nonfile_actions.then(|| Ok((checkpoint_batch, false)));
Ok(sidecar_content...chain(top_iterable))

... where need_nonfile_actions comes from a schema test, similar but opposite to the skip_sidecar_search. But it's probably not worth the complexity.

I agree, I'd like to move forward with unconditionally including the checkpoint batch for simplicities sake

I'd say either keep the separate function (if needed for testing) or embed the logic directly in the map call? What purpose does a separately named closure serve?

(aside: not sure if cargo fmt will like my indentation choice above -- depends on whether the ( or { is more important)

@scovich this is the batch creation I mentioned in the other comment.

Previously, when passing a schema that included the sidecar action to add_batch_simple, I found that the SidecarVisitor would find sidecar actions which had the empty path field

Fixed by #692, right?